Support SLIP-132 descriptor imports - #13
Conversation
Normalize SLIP-132 public keys embedded in descriptor strings before parsing them with Miniscript. This keeps canonical output as xpub or tpub while accepting descriptor exports that use ypub, zpub, upub, or vpub.
|
Warning Review limit reached
More reviews will be available in 34 minutes and 8 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. 📝 WalkthroughWalkthroughAdds a ChangesSLIP-132 normalization and descriptor parsing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR extends SLIP-132 normalization to raw descriptor strings, covering the case PR #9 left out: wallet exports that embed
Confidence Score: 5/5Safe to merge. The normalization is scoped to SLIP-132 prefixes, leaves canonical xpub/tpub descriptors untouched, and the checksum-strip path is gated behind actual key substitution. The Base58 alphabet in No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["Descriptor string input"] --> B["normalized_descriptor_line()"]
B --> C["normalize_slip132_public_keys()"]
C --> D{Any SLIP-132 key found?}
D -- "No (xpub/tpub)" --> E["Cow::Borrowed\n(unchanged string)"]
D -- "Yes (ypub/zpub/upub/vpub)" --> F["Replace each key via\nto_standard_extended_public_key()"]
F --> G["Cow::Owned\n(modified string)"]
E --> H["Return original line\n(checksum preserved)"]
G --> I{String contains '#'?}
I -- "Yes (checksum present)" --> J["rsplit_once('#')\nStrip invalidated checksum"]
I -- "No" --> K["Return normalized string\nwithout checksum"]
J --> L["Return stripped descriptor"]
H --> M["parse_descriptor_line()\nparse_descriptor(secp, line)"]
L --> M
K --> M
M --> N["Descriptor<DescriptorPublicKey>"]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A["Descriptor string input"] --> B["normalized_descriptor_line()"]
B --> C["normalize_slip132_public_keys()"]
C --> D{Any SLIP-132 key found?}
D -- "No (xpub/tpub)" --> E["Cow::Borrowed\n(unchanged string)"]
D -- "Yes (ypub/zpub/upub/vpub)" --> F["Replace each key via\nto_standard_extended_public_key()"]
F --> G["Cow::Owned\n(modified string)"]
E --> H["Return original line\n(checksum preserved)"]
G --> I{String contains '#'?}
I -- "Yes (checksum present)" --> J["rsplit_once('#')\nStrip invalidated checksum"]
I -- "No" --> K["Return normalized string\nwithout checksum"]
J --> L["Return stripped descriptor"]
H --> M["parse_descriptor_line()\nparse_descriptor(secp, line)"]
L --> M
K --> M
M --> N["Descriptor<DescriptorPublicKey>"]
Reviews (3): Last reviewed commit: "Skip short SLIP-132 tokens when normaliz..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/xpub.rs (1)
207-210: 💤 Low valueConsider adding a minimum token length check before conversion.
If a descriptor's checksum coincidentally starts with a SLIP-132 prefix (e.g.,
#zpubxxxx), the function would attempt to decode an 8-character token as an extended public key and fail. While extremely unlikely in practice (valid checksums are 8 chars; xpubs are 111 chars), adding a length guard would prevent false positives:if token.len() < 100 { // Not a valid xpub length, skip normalization for this token index += next_char.len_utf8(); continue; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/xpub.rs` around lines 207 - 210, The code currently attempts to convert any token matching the SLIP-132 prefix check without verifying it is long enough to be a valid extended public key. Add a length validation check after determining the token and before calling to_standard_extended_public_key to ensure the token is at least 100 characters long (the minimum length for a valid xpub). If the token is shorter than this minimum length, skip the normalization for that token and continue processing the next character by incrementing the index and using continue.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/xpub.rs`:
- Around line 207-210: The code currently attempts to convert any token matching
the SLIP-132 prefix check without verifying it is long enough to be a valid
extended public key. Add a length validation check after determining the token
and before calling to_standard_extended_public_key to ensure the token is at
least 100 characters long (the minimum length for a valid xpub). If the token is
shorter than this minimum length, skip the normalization for that token and
continue processing the next character by incrementing the index and using
continue.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 34468579-13c5-4b67-91f0-a5001c31c72b
📒 Files selected for processing (2)
src/descriptor.rssrc/xpub.rs
Add a minimum encoded extended public key length check and related safety improvements in src/xpub.rs. Introduce MIN_ENCODED_EXTENDED_PUBLIC_KEY_LENGTH (100) and skip base58 tokens shorter than that to avoid misinterpreting invalid short tokens (e.g. "zpubINVALID"). Replace an expect() on next character with safe pattern matching to prevent panics at string boundaries. Add a unit test (test_normalize_slip132_public_keys_skips_short_tokens) to verify short tokens are ignored.
1 participant
Summary
zpubkeys.Context
PR #9 added SLIP-132 support for bare keys, JSON, Electrum, Wasabi, and BIP380 key-expression imports. It did not cover the raw descriptor-string case where a wallet/export uses a non-canonical
ypub,zpub,upub, orvpubinside descriptor text.Canonical descriptors should still use standard BIP32
xpubortpubkeys, with the script wrapper carrying the script type. This change accepts those non-canonical descriptor imports for compatibility, normalizes them before parsing, and keeps descriptor output canonical.If normalization changes a descriptor that already has a checksum, the checksum is stripped before Miniscript parses the descriptor because replacing the embedded key invalidates the original checksum.
Validation
cargo fmtcargo clippycargo testSummary by CodeRabbit
New Features
Tests